Skip to content

Conversation

@stanpapa
Copy link

@stanpapa stanpapa commented Dec 1, 2025

This PR adds AbsDiffEq, RelativeEq and UlpsEq from the approxim crate for IndexMap. The trait implementation are locked behind the approxim feature flag.

These traits build on top of PartialEq and are used to compare values with variable precision, which simplifies float comparisons.

@cuviper
Copy link
Member

cuviper commented Dec 1, 2025

This seems like a quite niche thing to add, in part because approxim has much less usage than the original approx crate, yet we don't have that implementation either.

These traits also don't seem to be container-oriented -- they're not implemented for the standard BTreeMap or HashMap, nor even Vec. They are implemented for slices, but I'd guess that's more simd-oriented. What is the use case where you need an IndexMap itself to implement these traits?

Without any new dependencies, I could imagine having a generic comparison method, something like:

fn eq_by(&self, other: &IndexMap<K, V2, S2>, eq: impl FnMut(&V, &V2)) -> bool;

Would that meet your needs?

@stanpapa
Copy link
Author

stanpapa commented Dec 3, 2025

Thanks for taking the time and providing feedback so quickly!

This seems like a quite niche thing to add, in part because approxim has much less usage than the original approx crate, yet we don't have that implementation either.

I agree it is quite niche. The reason for choosing approxim instead of approx is because approx does not seem to be maintained anymore and approxim has more functionality than approx.

These traits also don't seem to be container-oriented -- they're not implemented for the standard BTreeMap or HashMap, nor even Vec. They are implemented for slices, but I'd guess that's more simd-oriented. What is the use case where you need an IndexMap itself to implement these traits?

My use case is that I would like to compare collections of molecular geometries (which are just lists of 3D coordinates). These collections are part of a bigger struct and to reduce boilerplate, we would like to derive these traits. Having IndexMap implement these traits makes that much cleaner.

Without any new dependencies, I could imagine having a generic comparison method, something like:

fn eq_by(&self, other: &IndexMap<K, V2, S2>, eq: impl FnMut(&V, &V2)) -> bool;

Would that meet your needs?

That would already simplify things somewhat. Were you thinking of something likes this:

    fn eq_by<V2, S2>(&self, other: &IndexMap<K, V2, S2>, eq: impl Fn(&V, &V2) -> bool) -> bool
    where
        S2: BuildHasher,
    {
        self.len() == other.len()
            && self
                .iter()
                .all(|(key, value)| other.get(key).map_or(false, |v| eq(value, v)))
    }

Given that my request is admittedly too niche for this crate, I will close this PR and try to have the implementation added to approxim itself, which might make more sense in this case.

Would you like me to open a new PR for the eq_by function or would you rather do it yourself?

@cuviper
Copy link
Member

cuviper commented Dec 4, 2025

That would already simplify things somewhat. Were you thinking of something likes this:

    fn eq_by<V2, S2>(&self, other: &IndexMap<K, V2, S2>, eq: impl Fn(&V, &V2) -> bool) -> bool
    where
        S2: BuildHasher,

I did intend FnMut in my original suggestion -- that allows the caller to have mutable state if they like, e.g. for caching. We should probably make that an explicit F type parameter as well, as I don't think we use impl Trait in public API anywhere. The only other design question I have is whether to pass the keys to this closure as well, even though we "know" they should be equal already in the normal sense due to other.get(key).

We could also allow different key types, given K: Equivalent<K2>. Even the PartialEq could allow that, but I don't remember whether we ever considered that.

Would you like me to open a new PR for the eq_by function or would you rather do it yourself?

If you're interested, feel free!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants